Review Summary

The approach is correct - revoke-token was the only hard-delete command missing the --force / confirmation safety pattern, and the fix is consistent with how the rest of the codebase handles this.

Two things worth addressing before merge:

1. Incomplete standardization in keys/revoke.ts

The PR description says it replaced interactiveHelper.confirm() with promptForConfirmation() to match all other destructive commands, but keys/revoke.ts line 113 still uses the old helper for the remove-from-config prompt. This is now the only remaining interactiveHelper.confirm() across all destructive commands. If the intent was full standardization, migrate this one too. If it was intentionally left, the PR description should clarify why.

2. Test cleanup not in try-finally - stdin can leak between tests

The 'should cancel when user declines confirmation' test (line 193) manipulates process.stdin but the cleanup is not in a try-finally block. If either expect() assertion throws an AssertionError, process.stdin stays replaced for every subsequent test in the same worker, causing silent test pollution. Move the Object.defineProperty restore and nock.cleanAll() into a finally block wrapping runCommand and the assertions.

Pull request overview

Adds the standard “destructive action safety” pattern to auth revoke-token and aligns auth keys revoke with the shared confirmation-prompt helper, with corresponding unit test updates.

Changes:

• Added --force flag + interactive confirmation prompt to auth revoke-token, including a JSON-mode guard that requires --force.
• Switched auth keys revoke confirmation from interactiveHelper.confirm() to promptForConfirmation() for consistency.
• Updated/extended unit tests to pass --force where required and to cover JSON guard + user-cancel flows.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Review: fix(auth): add --force flag to revoke-token, standardize keys revoke prompt

The core change is correct and necessary. Adding --force to auth revoke-token was the right fix — it was the only hard-delete command missing this safety pattern. The JSON-mode guard and interactive confirmation flow are implemented correctly and mirror what other destructive commands do.

Two issues worth addressing:

1. Incomplete standardization in keys/revoke.ts — second interactiveHelper.confirm() left unchanged

The PR standardizes the primary revocation prompt (line ~76) to promptForConfirmation(), but the secondary prompt at line ~112 still uses this.interactiveHelper.confirm() (inquirer):

// Line ~112 — unchanged in this PR
const shouldRemove = await this.interactiveHelper.confirm(
  "The revoked key was your current key for this app. Remove it from configuration?",
);

interactiveHelper.confirm uses inquirer, while promptForConfirmation uses readline. These behave differently in non-TTY environments — inquirer may throw or hang, while readline degrades gracefully. In a single command run, the user now hits two different prompt UX implementations back-to-back. If the stated goal is to standardize on promptForConfirmation, this second call should be updated too.

2. Cancellation test uses fragile stdin monkey-patching

it("should cancel when user declines confirmation", async () => {
  // replaces process.stdin with a Readable and pushes "n\n" via queueMicrotask
  mockStdinAnswer("n");
  ...
  const { stderr } = await runCommand([...]);
  expect(stderr).toContain("Revocation cancelled");
  expect(revokeNock.isDone()).toBe(false);
});

This works due to Node.js Readable stream buffering (data is pushed before readline attaches), but it is fragile: any refactor of promptForConfirmation (e.g., switching to inquirer or a different input mechanism) would silently break this test without a compile error. The idiomatic approach is to mock the function itself:

import * as promptModule from "../../../utils/prompt-confirmation.js";
vi.spyOn(promptModule, "promptForConfirmation").mockResolvedValueOnce(false);

This makes the test's intent explicit and decouples it from the implementation details of how stdin is read.

Both issues are fixable before merge. The feature itself is correct.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Review: [DX-1123] fix(auth): add --force flag to revoke-token, standardize keys revoke prompt

The approach is solid — adding the --force guard pattern to revoke-token and migrating all interactiveHelper.confirm() calls to promptForConfirmation() are both correct moves. Two issues below.

1. Component string inconsistency in revoke-token.ts

File: src/commands/auth/revoke-token.ts

The new JSON-mode guard uses "revokeToken" as the component string, but every other this.fail() call in the same file uses "tokenRevoke":

// New guard (PR adds this)
this.fail(
  "The --force flag is required when using --json to confirm revocation",
  flags,
  "revokeToken",  // ← doesn't match the rest of this file
);

// Existing calls (all use "tokenRevoke")
this.fail("Invalid API key format...", flags, "tokenRevoke");
this.fail("Token not found or already revoked", flags, "tokenRevoke");
this.fail(error, flags, "tokenRevoke");

Component strings appear in verbose log output and JSON error envelopes, so having two different component names for the same command makes debugging harder. Should be "tokenRevoke" to match.

2. Fragile stdin mock in the cancellation test

File: test/unit/commands/auth/revoke-token.test.ts

The "should cancel when user declines confirmation" test replaces process.stdin via Object.defineProperty and uses queueMicrotask to inject "n\n":

Object.defineProperty(process, "stdin", {
  value: readable,
  writable: true,
  configurable: true,
});
queueMicrotask(() => {
  for (const chunk of [`${answer}\n`, null]) readable.push(chunk);
});

Two risks:

• Replaceability: process.stdin is a getter-defined property in Node.js. In some environments (Node.js version, Vitest worker config) it may not be configurable, causing Object.defineProperty to throw silently or loudly. If it fails, the real stdin stays active and readline.question waits indefinitely, hanging the test.
• Timing: Pushing both "n\n" and null (EOF) via a single microtask means the stream is ended before readline's interface is created. If readline sees an already-ended stream, the callback might receive an empty string rather than "n" — cancellation would still happen, but for the wrong reason, masking a broken test.

A more reliable approach is vi.mock:

import { vi } from "vitest";

// At top of the file, before the describe block:
vi.mock("../../utils/prompt-confirmation.js", () => ({
  promptForConfirmation: vi.fn(),
}));

// In the test:
import { promptForConfirmation } from "../../utils/prompt-confirmation.js";
(promptForConfirmation as ReturnType<typeof vi.fn>).mockResolvedValueOnce(false);

This is immune to TTY/stdin environment differences and doesn't touch global process state.

Everything else looks correct: the --force flag is wired up properly, the JSON guard fires before the prompt block, examples are updated, promptForConfirmation is imported in all affected files (base-command.ts, keys/revoke.ts, bench/publisher.ts), and all existing tests correctly receive --force to bypass the new prompt.

Review

This is a clean, well-reasoned fix. The old command was genuinely broken — silently using a raw token string as a clientId is wrong at the API level — and this PR correctly redesigns the interface around what the revocation API actually supports.

What works well

• Removes a real semantic bug (token string ≠ clientId) and replaces it with correct, documented behavior
• Mutual exclusion between --client-id and --revocation-key is correctly wired via oclif's exclusive option
• Component strings standardized to "revokeToken" throughout
• Dead code removed (the createAblyRealtimeClient call was never used)
• promptForConfirmation standardization across base-command.ts, keys/revoke.ts, and bench/publisher.ts is correct
• AGENTS.md updated with the forceFlag + promptForConfirmation pattern — good for future contributors

Issues

1. JSON mode guard fires after ensureAppAndKey() — fail-fast ordering

revoke-token.ts (run() method):

const appAndKey = await this.ensureAppAndKey(flags);  // ← fetches config first
if (!appAndKey) return;

const { apiKey } = appAndKey;

// JSON mode guard comes here
if (!flags.force && this.shouldOutputJson(flags)) {
  this.fail("The --force flag is required when using --json ...", ...);
}

Compare with keys/revoke.ts, which correctly places its JSON mode guard before the API call (after displaying info). For revoke-token.ts, the target info is available purely from flags — there's nothing to fetch before showing the confirmation context. The guard should move above ensureAppAndKey():

// Fail fast — no need to fetch config first
if (!flags.force && this.shouldOutputJson(flags)) {
  this.fail("The --force flag is required when using --json ...", ...);
}

const appAndKey = await this.ensureAppAndKey(flags);

This isn't a crash, but it means a user running ably auth revoke-token --client-id foo --json gets a config lookup before seeing the error. Minor, but easy to fix.

2. Stdin patching in the cancellation test is environment-fragile

test/unit/commands/auth/revoke-token.test.ts, mockStdinAnswer:

function mockStdinAnswer(answer: string) {
  const readable = new Readable({ read() {} });
  Object.defineProperty(process, "stdin", {
    value: readable,
    writable: true,
    configurable: true,
  });
  queueMicrotask(() => {
    for (const chunk of [`${answer}\n`, null]) readable.push(chunk);
  });
}

Two concrete risks:

• process.stdin is not always configurable (Vitest worker threads, Node.js version differences). If Object.defineProperty fails silently, readline waits on real stdin and the test hangs indefinitely.
• Pushing the EOF (null) in the same microtask as the data means the stream is already closed before readline's question() callback can observe it. The readline interface receives an empty string on EOF, so the test still passes (empty string → false → cancelled), but for the wrong reason — it would pass even if the prompt was never shown.

The clean fix is to mock the module instead:

import { vi } from "vitest";
vi.mock("../../../utils/prompt-confirmation.js", () => ({
  promptForConfirmation: vi.fn(),
}));

// In the test:
import { promptForConfirmation } from "../../../utils/prompt-confirmation.js";
vi.mocked(promptForConfirmation).mockResolvedValueOnce(false);

This avoids global process state entirely and is immune to environment differences. The Copilot review raised the same concern — worth addressing before merge.

Both issues are fixable without structural changes. The semantic fix to revoke-token is correct and needed.

Review: auth revoke-token redesign

This is a solid, well-motivated fix. The old command was semantically broken — it accepted a token string but the Ably API does not support revoking by token value. The redesign correctly models the actual API with --client-id / --revocation-key and adds the --force safety pattern that was missing from this command.

What looks good

• Semantics are now correct — --client-id maps to clientId: and --revocation-key maps to revocationKey: in the request body, matching the Ably revocation API exactly.
• Mutual exclusion via exclusive: [...] — clean, idiomatic oclif approach for flags that cannot coexist.
• Force/JSON guard ordering — failing before ensureAppAndKey on the force check is the right call; it gives fast, actionable feedback in scripted mode.
• Dead code removal — the unused createAblyRealtimeClient call was a real problem: it created a persistent connection that was never used or cleaned up.
• Component string standardised to "revokeToken" across all this.fail() calls — was inconsistently "tokenRevoke" before.
• --allow-reauth-margin correctly maps to the API field allowReauthMargin.
• Test coverage — the new tests cover the happy path for both target types, the JSON mode guard, and cancellation. The stdin mock approach (Readable + queueMicrotask) is reasonable for testing readline-based prompts.

One real issue

revoke-token.ts: confirmation preamble uses this.log() (stdout) instead of this.logToStderr()

The interactive confirmation block uses this.log() to print "You are about to revoke all tokens matching:" and the target label/value before the prompt. Per the project convention, this.log() is for data/results on stdout; status/contextual text should go to stderr.

Practical impact: if someone pipes stdout (e.g. ably auth revoke-token --client-id foo | tee log.txt), these lines appear in the captured pipe as if they were structured output. The guard condition (!flags.force && !this.shouldOutputJson(flags)) means this only runs in interactive terminal mode, so real-world impact is low — but it is still a deviation from the established pattern.

Fix: replace both this.log() calls with this.logToStderr().

Minor observations (not blocking)

• Empty afterEach: The top-level afterEach now has no body after removing nock.cleanAll(). Harmless but dead code — could be deleted.
• Describe block names: The test uses "flag validation" and "token revocation by client ID/key" instead of the standard "argument validation" and "functionality" block names. Does not affect correctness.

Summary

The redesign is correct and meaningfully improves safety and semantic accuracy. One real issue: the two this.log() preamble lines before the confirmation prompt should be this.logToStderr(). Everything else looks well done.

PR Review: auth revoke-token redesign + confirmation pattern migration

Summary

The core semantic change is correct — the Ably revocation API targets clientId: or revocationKey: specifiers, not token strings, and the old positional token arg was misleading. Removing the dead createAblyRealtimeClient call and fixing the inconsistent component string (tokenRevoke → revokeToken) are both good cleanup.

Issues

1. Pre-confirmation context uses this.log() (stdout) instead of stderr — minor convention violation

In the new interactive flow in revoke-token.ts:

this.log(`\nYou are about to revoke all tokens matching:`);
this.log(`${formatLabel(targetLabel)} ${formatResource(targetValue)}`);

this.log() writes to stdout. Per project convention, status/informational lines that aren't data output should go to stderr. The rest of the codebase uses this.logToStderr() for this kind of pre-action context (compare: keys/revoke.ts also uses this.log() for its "Key to revoke:" display — neither is stdout-safe).

The practical risk is low — this only runs in interactive (non-JSON, non-force) mode where scripts aren't parsing stdout. But if someone captures stdout for logging, they'll get these lines mixed with data.

Suggested fix:

this.logToStderr(`\nYou are about to revoke all tokens matching:`);
this.logToStderr(`${formatLabel(targetLabel)} ${formatResource(targetValue)}`);

2. --allow-reauth-margin default not surfaced in help — per-convention omission

The flag has default: false, but per project convention (AGENTS.md): "falsy defaults (0, false) — oclif suppresses these, so keep them in the description." The current description explains the positive behavior but a user running --help won't see that the margin is off by default.

Suggested fix: Add (default: disabled) or similar to the description so help output is self-contained.

What's correct

• JSON mode guard ordering: target validation → JSON+force guard → ensureAppAndKey → interactive prompt. Failing fast before the config lookup is right.
• exclusive on --client-id/--revocation-key: oclif enforces mutual exclusion natively — no custom validation needed. Clean.
• Non-null assertion (clientId ?? revocationKey)!: Safe — this.fail() returns never when both are null, so TypeScript needs the hint. Correct.
• Inner catch re-throw pattern: The token_not_found specific check + re-throw to outer catch is sound. The outer this.fail() handles the generic case.
• Component string fixed: "revokeToken" (camelCase, consistent) throughout.
• Test coverage: JSON mode guard, user cancellation (with stdin mock), revocation-key path, allowReauthMargin request body — all the important paths are covered. The stdin mock approach (queueMicrotask + Readable + process.stdin replacement) is the right pattern for readline-based prompts.

Both issues above are minor. No blocking concerns.